Refactor commit logic #2580

alcaeus · 2023-11-08T13:14:51Z

Q	A
Type	improvement
BC Break	no
Fixed issues

Summary

PHPORM-112

(This PR currently targets 2.6.x, but will be held back until we work on 2.7.x)

As is, failures during a commit operation in UnitOfWork result in an inconsistent state. While updates and deletions mostly handle errors during the operation gracefully, errors during an insert or upset result in the UnitOfWork having a broken state.

This PR changes the timing of when we mark changes as done and remove changesets. All of these are now handled after all commit operations have concluded, which is a change made in preparation for transaction support (which requires us to keep changesets around until we successfully commit the transaction or give up). As a side effect, this will eventually allow fixing the inconsistencies mentioned above. A new test class keeps track of these inconsistencies so they can be addressed in the future.

lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php

lib/Doctrine/ODM/MongoDB/UnitOfWork.php

alcaeus · 2023-11-08T13:23:57Z

tests/Doctrine/ODM/MongoDB/Tests/UnitOfWorkCommitConsistencyTest.php

+use MongoDB\BSON\ObjectId;
+use Throwable;
+
+class UnitOfWorkCommitConsistencyTest extends BaseTestCase


These are the tests I used to test original behaviour and make sure that refactoring didn't break anything. I then used the undesired behaviour (see first commit in this PR) as a guideline for fixing the persister class.

lib/Doctrine/ODM/MongoDB/UnitOfWork.php

alcaeus · 2023-11-13T14:17:54Z

After a number of unsuccessful attempts at getting non-transactional flushes to behave somewhat normal, I realised that this requires a lot more changes to UnitOfWork, DocumentPersister, and CollectionPersister than I'm willing to make at this point. I've kept the tests around, but changed the assertions to match current broken behaviour (which changes with this PR for some of the tests, but then you can't break something that isn't broken already).

Scheduled document changes and changesets are now cleared at the end of the commit operation only if the operation itself was successful. In other cases, no changes were made and a subsequent flush may want to persist data again. I'll try to address this once transactional commit is implemented, as that will give me a rough idea of how the behaviour differs.

lib/Doctrine/ODM/MongoDB/UnitOfWork.php

tests/Doctrine/ODM/MongoDB/Tests/UnitOfWorkCommitConsistencyTest.php

jmikola

A few optional suggestions to improve the test file, but changes LGTM.

jmikola · 2023-11-21T19:14:54Z

tests/Doctrine/ODM/MongoDB/Tests/BaseTestCase.php

+    {
+        $uri = getenv('DOCTRINE_MONGODB_SERVER') ?: DOCTRINE_MONGODB_SERVER;
+
+        return $useMultipleMongoses ? $uri : self::removeMultipleHosts($uri);


Expect this to fail horribly if we ever test with an SRV URI pointing to a sharded cluster.

Yep, I'm hoping it won't come to that. Please point me to this comment with an appropriate gif once the time comes.

jmikola · 2023-11-21T19:19:26Z

tests/Doctrine/ODM/MongoDB/Tests/UnitOfWorkCommitConsistencyTest.php

+        self::assertTrue($this->uow->isScheduledForDelete($user));
+    }
+
+    public function testFailingDeleteWithEmbeddedDocumentKeepsChangeset(): void


Since this is derivative of the previous test, I think the names could be more similar:

testDeleteFailureKeepsChangeset

testDeleteFailureWithEmbeddedDocumentKeepsChangeset

Something to that effect.

I've renamed all tests to be of the form test<Operation>(Error)?<Description>.

jmikola · 2023-11-21T19:20:24Z

tests/Doctrine/ODM/MongoDB/Tests/UnitOfWorkCommitConsistencyTest.php

+        self::assertTrue($this->uow->isScheduledForDelete($address));
+    }
+
+    public function testSuccessfulDeleteWithEmbeddedDocumentClearsChangeset(): void


Related to my previous comment, this could be:

testDeleteWithEmbeddedDocumentClearsChangeset

I don't think "Success" adds much value (it can be assumed).

jmikola · 2023-11-21T19:21:44Z

tests/Doctrine/ODM/MongoDB/Tests/UnitOfWorkCommitConsistencyTest.php

+        // Make sure delete command fails once
+        $this->dm->getClient()->selectDatabase('admin')->command([
+            'configureFailPoint' => 'failCommand',
+            'mode' => ['times' => 1],
+            'data' => [
+                'errorCode' => 192, // FailPointEnabled
+                'failCommands' => ['delete'],
+            ],
+        ]);


Consider a private helper method that just takes a command name to remove all of this duplication and make the tests more readable.

Good point.

jmikola · 2023-11-21T19:23:22Z

tests/Doctrine/ODM/MongoDB/Tests/UnitOfWorkCommitConsistencyTest.php

+    {
+        // Create a unique index on the collection to let the second insert fail
+        $collection = $this->dm->getDocumentCollection(ForumUser::class);
+        $collection->createIndex(['username' => 1], ['unique' => true]);


Why bother with a unique index when update and delete use fail points?

The commit call in this test will run insertMany with three documents. A fail point would make the insert command fail before writing the first document; however I specifically wanted to test the behaviour when the first document can be inserted but the second can't; the only way we can test this is by using an index error on the second document. I've expanded the comment.

This ensures that the fail points are created on the same server that the write operations take place on, which can't be guaranteed in a sharded cluster with multiple mongoses.

* Add tests for commit consistency showing wrong behaviour * Clear scheduled document changes at the end of a commit operation * Rename private variables to communicate intent * Remove obsolete comment * Use different error code * Use single mongos in consistency tests This ensures that the fail points are created on the same server that the write operations take place on, which can't be guaranteed in a sharded cluster with multiple mongoses. * Rename test methods for clarity * Explain reasoning for index error * Extract helper method to create failpoint

* Run CI workflows on feature branches * Refactor commit logic (#2580) * Add tests for commit consistency showing wrong behaviour * Clear scheduled document changes at the end of a commit operation * Rename private variables to communicate intent * Remove obsolete comment * Use different error code * Use single mongos in consistency tests This ensures that the fail points are created on the same server that the write operations take place on, which can't be guaranteed in a sharded cluster with multiple mongoses. * Rename test methods for clarity * Explain reasoning for index error * Extract helper method to create failpoint * Add configuration setting for transactional flush (#2587) * Add configuration setting for transactional flush * Use classic setters for transactional flush setting * Add logic for transactional commits (#2589) * Extract commit logic * Support transactional commit operations * Always use transactional flush if supported With this commit, all tests using the document manager use transactional flush as long as transactions are supported. Certain tests can use the static $allowsTransactions variable to disable this behaviour. * Test with MongoDB 7.0 * Update test names * Update phpstan baseline * Fix query selection in shard key tests * Flip transaction options constant by default * Use supportsTransaction method when skipping tests * Apply review feedback to tests * Add separate test to check write concern in commit options * Strip write options when in transaction * Use majority write concern in test * Update events to play nice with transactions (#2594) * Pass session and transaction information to event args * Only dispatch lifecycle events once per commit operation * Remove isInTransaction property in event args * Split method signature for readability * Use property promotion for event args classes * Extract construction of eventArgs * Inline spl_object_hash calls * Avoid injecting test instance * Add session to commitOptions in persister * Add session assertions in LifecycleEventManager * Only retry transaction once (#2604) * Only retry transaction once * Rename variable * Update transaction documentation (#2606) * Remove references to transactions where not applicable * Update transaction documentation * Apply suggestions from code review Co-authored-by: Jeremy Mikola <jmikola@gmail.com> * Change title level in event documentation * Add documentation about transactions to event docs --------- Co-authored-by: Jeremy Mikola <jmikola@gmail.com> * Address code review items * Test highest dependencies on MongoDB 7.0 * Regenerate psalm baseline to silence unrelated errors --------- Co-authored-by: Jeremy Mikola <jmikola@gmail.com>

alcaeus added this to the 2.7.0 milestone Nov 8, 2023

alcaeus requested review from jmikola and malarzm November 8, 2023 13:14

alcaeus self-assigned this Nov 8, 2023

alcaeus force-pushed the transactions/refactor-commit-logic branch from 1459426 to b2aee6a Compare November 8, 2023 13:17

alcaeus commented Nov 8, 2023

View reviewed changes

GromNaN reviewed Nov 8, 2023

View reviewed changes

lib/Doctrine/ODM/MongoDB/UnitOfWork.php Outdated Show resolved Hide resolved

alcaeus marked this pull request as draft November 9, 2023 13:33

alcaeus removed request for jmikola and malarzm November 9, 2023 13:34

alcaeus force-pushed the transactions/refactor-commit-logic branch from b2aee6a to 78e8744 Compare November 13, 2023 14:13

alcaeus marked this pull request as ready for review November 13, 2023 14:17

alcaeus force-pushed the transactions/refactor-commit-logic branch from 78e8744 to 9f34b4c Compare November 13, 2023 14:20

jmikola reviewed Nov 13, 2023

View reviewed changes

alcaeus requested a review from jmikola November 14, 2023 13:20

jmikola approved these changes Nov 21, 2023

View reviewed changes

alcaeus changed the base branch from 2.6.x to feature/transactions November 24, 2023 08:09

alcaeus force-pushed the transactions/refactor-commit-logic branch from 2e919a5 to 88626f6 Compare November 24, 2023 08:09

alcaeus added 9 commits November 24, 2023 09:15

Add tests for commit consistency showing wrong behaviour

932fdb0

Clear scheduled document changes at the end of a commit operation

baf7805

Rename private variables to communicate intent

05c9cd0

Remove obsolete comment

3af7d7c

Use different error code

5eaa8c1

Use single mongos in consistency tests

5dcd722

This ensures that the fail points are created on the same server that the write operations take place on, which can't be guaranteed in a sharded cluster with multiple mongoses.

Rename test methods for clarity

829557c

Explain reasoning for index error

b6f7647

Extract helper method to create failpoint

e53cbb6

alcaeus force-pushed the transactions/refactor-commit-logic branch from 88626f6 to e53cbb6 Compare November 24, 2023 08:15

alcaeus merged commit 13e5ca2 into doctrine:feature/transactions Nov 24, 2023
16 checks passed

alcaeus deleted the transactions/refactor-commit-logic branch November 24, 2023 08:29

alcaeus mentioned this pull request Nov 24, 2023

Add logic for transactional commits #2589

Merged

alcaeus removed this from the 2.7.0 milestone Jan 17, 2024

alcaeus mentioned this pull request Jan 17, 2024

Transaction support #2586

Merged

franmomu mentioned this pull request Mar 24, 2024

Run tests without doctrine/annotations installed doctrine-extensions/DoctrineExtensions#2781

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor commit logic #2580

Refactor commit logic #2580

alcaeus commented Nov 8, 2023 •

edited

Loading

alcaeus Nov 8, 2023

alcaeus commented Nov 13, 2023

jmikola left a comment

jmikola Nov 21, 2023

alcaeus Nov 23, 2023

jmikola Nov 27, 2023

jmikola Nov 21, 2023

alcaeus Nov 23, 2023

jmikola Nov 21, 2023

jmikola Nov 21, 2023

alcaeus Nov 23, 2023

jmikola Nov 21, 2023

alcaeus Nov 23, 2023

Refactor commit logic #2580

Refactor commit logic #2580

Conversation

alcaeus commented Nov 8, 2023 • edited Loading

Summary

Choose a reason for hiding this comment

alcaeus commented Nov 13, 2023

jmikola 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

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

alcaeus commented Nov 8, 2023 •

edited

Loading